Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import YAML with context #288

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lucianovici
Copy link

Import with context to avoid errors like:

[CRITICAL] Rendering SLS ... failed: Jinja variable 'grains' is undefined
[CRITICAL] Rendering SLS ... failed: Jinja variable 'salt' is undefined

Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're still deciding whether to use this method, please amend and rebase this PR as suggested below, so that it's showing all tests passing and ready for merging.

Comment on lines +5 to +11
{%- import_yaml tplroot ~ "/defaults.yaml" as default_settings with context %}
{%- import_yaml tplroot ~ "/osfamilymap.yaml" as osfamilymap with context %}
{%- import_yaml tplroot ~ "/osfingermap.yaml" as osfingermap with context %}
{%- import_yaml tplroot ~ "/osmap.yaml" as osmap with context %}
{%- import_yaml tplroot ~ "/codenamemap.yaml" as codenamemap with context %}
{%- import_yaml tplroot ~ "/osarchmap.yaml" as osarchmap with context %}
{%- import_yaml tplroot ~ "/cpuarchmap.yaml" as cpuarchmap with context %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lucianovici Grepping through the .yaml files, these are the only ones that are using any Jinja for accessing data structures:

docker/osfamilymap.yaml:{%- if grains.os == 'MacOS' %}
docker/osfamilymap.yaml:    {%- set rootuser = salt['cmd.run']("stat -f '%Su' /dev/console") %}
docker/osfamilymap.yaml:    {%- set rootgroup = salt['cmd.run']("stat -f '%Sg' /dev/console") %}
docker/osfamilymap.yaml:{%- elif grains.os == 'Windows' %}
docker/osfamilymap.yaml:    {%- set rootuser = salt['cmd.run']("id -un") %}
docker/osfamilymap.yaml:{%- endif %}
docker/osfamilymap.yaml:                {%- if 'oscodename' in grains %}
docker/osfamilymap.yaml:        name: deb [arch=amd64] https://download.docker.com/linux/{{ grains.os|lower }} {{ grains.oscodename }} stable
docker/osfamilymap.yaml:                {%- endif %}
docker/osfamilymap.yaml:        key_url: "https://download.docker.com/linux/{{ grains.os|lower }}/gpg"
docker/osfamilymap.yaml:        baseurl: 'https://download.docker.com/linux/{{ grains.os|lower }}/$releasever/$basearch/stable'
docker/osfamilymap.yaml:        gpgkey: 'https://download.docker.com/linux/{{ grains.os|lower }}/gpg'
docker/osfamilymap.yaml:    rootuser: {{ rootuser | d('') }}
docker/osfamilymap.yaml:    rootuser: {{ rootuser | d('') }}
docker/codenamemap.yaml:      - linux-image-extra-{{ grains.kernelrelease }}
docker/osmap.yaml:        baseurl: 'https://download.docker.com/linux/centos/{{ grains.get('osmajorrelease', '') }}/$basearch/stable'

Thus, we only need to add with context to those particular imports (as was mentioned in the postgres-formula PR):

Suggested change
{%- import_yaml tplroot ~ "/defaults.yaml" as default_settings with context %}
{%- import_yaml tplroot ~ "/osfamilymap.yaml" as osfamilymap with context %}
{%- import_yaml tplroot ~ "/osfingermap.yaml" as osfingermap with context %}
{%- import_yaml tplroot ~ "/osmap.yaml" as osmap with context %}
{%- import_yaml tplroot ~ "/codenamemap.yaml" as codenamemap with context %}
{%- import_yaml tplroot ~ "/osarchmap.yaml" as osarchmap with context %}
{%- import_yaml tplroot ~ "/cpuarchmap.yaml" as cpuarchmap with context %}
{%- import_yaml tplroot ~ "/defaults.yaml" as default_settings %}
{%- import_yaml tplroot ~ "/osfamilymap.yaml" as osfamilymap with context %}
{%- import_yaml tplroot ~ "/osfingermap.yaml" as osfingermap %}
{%- import_yaml tplroot ~ "/osmap.yaml" as osmap with context %}
{%- import_yaml tplroot ~ "/codenamemap.yaml" as codenamemap with context %}
{%- import_yaml tplroot ~ "/osarchmap.yaml" as osarchmap %}
{%- import_yaml tplroot ~ "/cpuarchmap.yaml" as cpuarchmap %}

In fact, this is actually a reminder that we really want to move to our new v5 map.jinja, which emphasizes moving Jinja out of our YAML files.

Also, #290 has been merged, so if you rebase this PR on top, all the tests should pass.

@myii
Copy link
Member

myii commented May 19, 2021

@Lucianovici When amending the commit, please update the commit message for semantic-release purposes:

-Import YAML with context
+fix(map.jinja): import YAML with context

@myii
Copy link
Member

myii commented May 19, 2021

@danny-smit I see you have some PRs active for this formula, which would benefit from this fix being applied. How about we get this PR merged soon, as one example of what would need to be done in case we're forced to patch the 30+ formulas?

@danny-smit
Copy link
Collaborator

@myii Thanks, that sounds very good to me. Can I help with that?

@myii
Copy link
Member

myii commented May 19, 2021

@myii Thanks, that sounds very good to me. Can I help with that?

@danny-smit Ah, I've just noticed that this PR was started from the contributor's master branch, so I don't think I can do the usual process of amending the PR myself. There are other ways using git but those don't work well on the GitHub side of things. Let's hope @Lucianovici is able to adjust the PR soon, so that we can get it merged.

@myii
Copy link
Member

myii commented May 19, 2021

@danny-smit Actually, are you able to commit my suggestion from the review above? If so, I can finish the rest upon squash-merging this PR.

@danny-smit
Copy link
Collaborator

@myii No, I don't have permission to add commits.

@myii
Copy link
Member

myii commented May 19, 2021

@myii No, I don't have permission to add commits.

I've been chatting to @javierbertoli -- we're thinking about rebuilding the master images to the last commit before this problem started, keeping it there until there's some clarity from the Salt devs. If we do that, we can simply rerun the CI on your PRs and they should just work.

@myii
Copy link
Member

myii commented May 19, 2021

@danny-smit So we've started fixing the images and most are working now. You can see the CI re-run of #289 here:

It's actually wider-reaching than just the master builds. It's actually all the builds installing Salt using git (i.e. pip), so I'm in the process of fixing those as well.

@danny-smit
Copy link
Collaborator

@myii Nice work. The tests of the CI re-runs are working much better now!

Using releases from github instead of bintray
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants